fix(menu): Fixed maxHeight calculation for menu portal. #5716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the viewSpaceBelow is less than 300px (default max height): The first time the menu renders, the viewSpaceBelow will probably not be greater than the menuHeight. So it will take path 2. or 3. for bottom/auto. If the viewable area is greater than the minimum height then it will take path 3. On the next render, the menuHeight is now less than viewSpaceBelow because it was constrained by the third path. So the first path is taken, but preferredMaxHeight will be greater than viewSpaceBelow because path 3 was taken on the previous render. The menu renders but overflows outside the view. On the following render, path 1 is not taken because the height is greater than the viewSpaceBelow again (because its overflowing). So path 3 is taken again. This resolves the potential for flickering by making sure that the size that _would_ be rendered will actually fit. But comparing the `viewSpaceBelow` and `scrollSpaceBelow` to the `maxHeight` that would actually be returned, not to the height of the menu currently.
🦋 Changeset detectedLatest commit: 1b3571b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1b3571b:
|
BugEach hover causes a re-render, causing the height to be recalculated. react-select-bug-2023-07-30_17.22.25.mp4PatchedThe menu will not be larger than the space available. react-select-fixed-2023-07-30_17.23.48.mp4 |
jjuradok
approved these changes
Aug 7, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the viewSpaceBelow is less than 300px (default max height):
The first time the menu renders, the viewSpaceBelow will probably not be greater than the menuHeight. So it will take path 2. or 3. for bottom/auto. If the viewable area is greater than the minimum height then it will take path 3.
On the next render, the menuHeight is now less than viewSpaceBelow because it was constrained by the third path. So the first path is taken, but preferredMaxHeight will be greater than viewSpaceBelow because path 3 was taken on the previous render. The menu renders but overflows outside the view.
On the following render, path 1 is not taken because the height is greater than the viewSpaceBelow again (because its overflowing). So path 3 is taken again.
This resolves the potential for flickering by making sure that the size that would be rendered will actually fit. But comparing the
viewSpaceBelowandscrollSpaceBelowto themaxHeightthat would actually be returned, not to the height of the menu currently.TLDR; When the space the menu would be rendered in is less than the maxHeight, but greater than the minHeight; the menu will flicker because it alternates the maxHeight value because of how it checks space available. This changes it so that the check is against what the new height would be, instead of against what it is now.
If needed I can provide a video of the behavior; its absolutely possible I'm doing something wrong here. But the fact that
menuHeightcould be less thanpreferredMaxHeightseems like this is still a bug.